Repository navigation
fix: filter terminal query responses from scrollback on reattach - #154
Conversation
When reattaching to a terminal, escape sequence responses (CPR, device attributes, OSC color queries) were being displayed as garbage text. This filters them from scrollback storage while preserving them for live xterm.js processing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Rate limit exceeded@Kitenite has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughIntroduces terminal query response filtering for scrollback persistence. A new Changes
Sequence DiagramsequenceDiagram
participant PTY as PTY Output
participant TM as TerminalManager
participant UI as Terminal UI
participant SB as Scrollback/History
PTY->>TM: Raw data (with escape sequences)
alt Data Processing
TM->>UI: Emit raw data unfiltered
TM->>TM: filterTerminalQueryResponses()
TM->>SB: Store filtered data
end
Note over TM,SB: Query responses (CPR, DA, OSC) removed<br/>before persistence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/terminal-manager.ts (1)
20-37: Consider pre-compiling regex patterns for better performance.The regex patterns are recreated on every function call. While the static analysis tool flagged a potential ReDoS vulnerability, the risk is low since the input comes from controlled PTY output. However, pre-compiling the patterns outside the function would improve performance and address the static analysis concern.
Apply this diff to pre-compile the patterns:
+// Pre-compiled patterns for filtering terminal query responses +const CSI_QUERY_PATTERN = new RegExp( + `${ESC}\\[\\??[\\d;]*[Rc]|${ESC}\\[\\?\\d+;\\d+\\$y`, + "g", +); +const OSC_QUERY_PATTERN = new RegExp( + `${ESC}\\]1[0-9];rgb:[0-9a-fA-F]{4}/[0-9a-fA-F]{4}/[0-9a-fA-F]{4}(?:${BEL}|${ESC}\\\\)`, + "g", +); + /** * Filters out terminal query responses from PTY output before storing in scrollback. * These are responses to xterm.js queries that shouldn't be replayed on reattach: * - Cursor Position Reports (CPR): ESC [ row ; col R * - Device Attributes (DA): ESC [ ? Ps c * - OSC color responses: ESC ] 10;rgb:... or ESC ] 11;rgb:... etc. * - DECRPM responses: ESC [ ? Ps ; Pm $ y */ function filterTerminalQueryResponses(data: string): string { - // Cursor Position Report: ESC[<row>;<col>R - // Device Attributes: ESC[?<params>c or ESC[<params>c - // DECRPM: ESC[?<param>;<mode>$y - const csiPattern = new RegExp( - `${ESC}\\[\\??[\\d;]*[Rc]|${ESC}\\[\\?\\d+;\\d+\\$y`, - "g", - ); - - // OSC responses for colors (10=fg, 11=bg, 12=cursor, etc.): ESC]<num>;rgb:...<ST> - // String terminator can be BEL or ST (ESC\) - const oscPattern = new RegExp( - `${ESC}\\]1[0-9];rgb:[0-9a-fA-F]{4}/[0-9a-fA-F]{4}/[0-9a-fA-F]{4}(?:${BEL}|${ESC}\\\\)`, - "g", - ); - - return data.replace(csiPattern, "").replace(oscPattern, ""); + return data.replace(CSI_QUERY_PATTERN, "").replace(OSC_QUERY_PATTERN, ""); }Note: Based on static analysis hints, the tool flagged potential ReDoS concerns. While the risk is minimal given the controlled input source, pre-compilation addresses both performance and the static analysis warning.
apps/desktop/src/main/lib/terminal-manager.test.ts (1)
511-566: Test comprehensively validates filtering behavior.The test effectively covers the escape sequence filtering feature, verifying both raw data emission and filtered scrollback recovery. However, per coding guidelines stating "Tests should have one assert per test," consider splitting this into separate test cases for easier debugging.
Consider splitting into focused tests:
Test 1: "should emit raw data including escape sequences to xterm"
- Create session and send data with escape sequences
- Assert raw data is emitted with escape sequences
Test 2: "should filter escape sequences from scrollback persistence"
- Create session, send data with escape sequences, trigger exit
- Assert recovered scrollback contains only filtered content
This would make test failures easier to diagnose while maintaining the same coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/main/lib/terminal-manager.test.ts(1 hunks)apps/desktop/src/main/lib/terminal-manager.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Apply Biome formatting, linting, and import organization at the root level for all code files
Files:
apps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/main/lib/terminal-manager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype; use type-safe approaches instead, unless necessary
Ensure TypeScript type checking passes across all packages using bun run typecheck
Files:
apps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/main/lib/terminal-manager.ts
apps/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
apps/**/*.{tsx,ts}: Structure React applications with one folder per component following the pattern: ComponentName/ComponentName.tsx with index.ts barrel export
Co-locate component dependencies (hooks, utils, constants, tests, stories) next to the file using them
Files:
apps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/main/lib/terminal-manager.ts
apps/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate tests with their corresponding components (e.g., Component.test.tsx next to Component.tsx)
Files:
apps/desktop/src/main/lib/terminal-manager.test.ts
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/main/lib/terminal-manager.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/main/lib/terminal-manager.ts
apps/desktop/**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.test.{ts,tsx,js,jsx}: Tests should have one assert per test
Tests should be readable
Tests should be fast
Tests should be independent
Tests should be repeatable
Files:
apps/desktop/src/main/lib/terminal-manager.test.ts
🧠 Learnings (2)
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be repeatable
Applied to files:
apps/desktop/src/main/lib/terminal-manager.test.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/main/lib/*-ipcs.ts : Implement Electron IPC handlers in dedicated files under apps/desktop/src/main/lib/ (e.g., workspace-ipcs.ts, terminal-ipcs.ts)
Applied to files:
apps/desktop/src/main/lib/terminal-manager.ts
🪛 ast-grep (0.40.0)
apps/desktop/src/main/lib/terminal-manager.ts
[warning] 23-26: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
${ESC}\\[\\??[\\d;]*[Rc]|${ESC}\\[\\?\\d+;\\d+\\$y,
"g",
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 30-33: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
${ESC}\\]1[0-9];rgb:[0-9a-fA-F]{4}/[0-9a-fA-F]{4}/[0-9a-fA-F]{4}(?:${BEL}|${ESC}\\\\),
"g",
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
apps/desktop/src/main/lib/terminal-manager.ts (2)
8-10: LGTM!Clean constant definitions that improve regex readability.
170-178: LGTM! Correct separation of filtered and raw data.The implementation correctly filters escape sequences before storing in scrollback and history, while still emitting the raw data to xterm.js for proper terminal processing. This achieves the stated goal of preventing garbage characters on reattach.
- Extract filterTerminalQueryResponses to terminal-escape-filter.ts - Add comprehensive test suite with 54 test cases covering: - Cursor Position Reports (CPR) including row-only format - Primary/Secondary Device Attributes (DA1/DA2) - Device Attributes without ? or > prefix - DEC Private Mode Reports (DECRPM) - OSC color responses (10-19) - Tertiary Device Attributes (DA3) - XTVERSION responses - Edge cases and mixed content - Simplify terminal-manager.test.ts to focus on integration 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
2R1R0;276;0c10;rgb:ffff/ffff/ffff...from appearing when reattaching to terminalsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.